chore(ci): fix Operator tests, artifact handling, pod selectors - #4414
Conversation
|
/test e2e-ocp-operator-nightly |
|
/test e2e-ocp-operator-nightly |
f421399 to
1de4c25
Compare
|
The container image build workflow finished with status: |
|
/test e2e-ocp-operator-nightly |
ebafb7a to
02958d9
Compare
|
/test e2e-ocp-operator-nightly |
02958d9 to
1851b53
Compare
|
/test e2e-ocp-operator-nightly |
|
/test e2e-ocp-operator-nightly |
1 similar comment
|
/test e2e-ocp-operator-nightly |
|
/agentic_review |
Code Review by Qodo
1. Crunchy wait omitted
|
|
/test e2e-ocp-operator-auth-providers-nightly |
|
@zdrapela: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/test e2e-eks-operator-nightly |
| const conditions = response.body.status?.conditions || []; | ||
|
|
||
| const podSelector = await this.getDeploymentPodSelector( | ||
| deploymentName, |
There was a problem hiding this comment.
This getDeploymentPodSelector call is inside the while (Date.now() < endTime) polling loop, so it makes an API call to read the deployment on every iteration. The deployment's matchLabels won't change between retries — should we resolve this once before the loop and reuse it?
There was a problem hiding this comment.
Good catch, let me move it
| @@ -658,8 +661,8 @@ export class KubeClient { | |||
| } | |||
| } | |||
There was a problem hiding this comment.
logPodConditions now requires labelSelector as a non-optional string, but podSelector here can be null (returned by getDeploymentPodSelector). The if (expectedReplicas > 0 && podSelector) guard on line 648 only covers checkPodFailureStates, not this call. Should this also be wrapped in an if (podSelector) check?
There was a problem hiding this comment.
True, I'll update the pod selector so it cannot be null.
| test.skip(() => skipIfJobName(JOB_NAME_PATTERNS.GKE)); // skipping orchestrator tests on GKE - plugins disabled | ||
| test.skip(() => skipIfJobName(JOB_NAME_PATTERNS.AKS)); // skipping orchestrator tests on AKS - plugins disabled | ||
| test.skip(() => skipIfJobName(JOB_NAME_PATTERNS.EKS)); // skipping orchestrator tests on EKS - plugins disabled | ||
| // TODO: https://issues.redhat.com/browse/RHDHBUGS-2184 fix orchestrator tests on Operator deployment |
There was a problem hiding this comment.
This block of 5 skip/fixme calls is now identical across orchestrator-entity-rbac.spec.ts, orchestrator-entity-workflows.spec.ts, and token-propagation-workflow.spec.ts. If another platform gets added you'd need to update all three. Would a shared helper like skipOrchestratorOnUnsupportedPlatforms() make sense here?
There was a problem hiding this comment.
There was a problem hiding this comment.
If there is no plan to enable Orchestrator tests on K8s platforms, I'd rather use shouldSkipOrchestratorTests in the playwright.config.ts as a much cleaner option.
The test.skip is good if we want to enable them in the future.
| local namespace=$1 | ||
| local backstage_crd_path=$2 | ||
|
|
||
| wait_for_crunchy_crd() { |
There was a problem hiding this comment.
The comment says this is only relevant "on OpenShift" but the function itself has no guard — it'll fail if ever called on a non-OCP cluster. The protection comes from call sites being in OCP-specific scripts, which is fine, but maybe add a brief note like # Caller must ensure this runs on OCP only to make the contract explicit?
There was a problem hiding this comment.
It's actually an AI comment from the codebase which wasn't correct. I'll update the description of the function to be more accurate
| // This works for both Helm and Operator since both set app.kubernetes.io/name | ||
| // on the Deployment (with different values), even though pod labels differ. | ||
| const deployTarget = `$(oc get deploy -n ${namespace} -l ${deploySelector} -o name)`; | ||
| let grepCommand = `oc logs ${deployTarget} --tail=${tailNumber} -c backstage-backend -n ${namespace}`; |
There was a problem hiding this comment.
Pre-existing, but since you're touching this line: filterWords is interpolated directly into a shell command via grep '${word}'. If any word contains a single quote, this breaks (or worse). Worth sanitizing or switching to a safer interpolation?
There was a problem hiding this comment.
As you said, it's pre-existing, I barely touched the line of code, and you're suggesting a change to a code that you introduced in #2827. I don't mean to be rude, but why don't you open a PR yourself to fix it? 🤷
There was a problem hiding this comment.
@zdrapela sorry I was just trying to take the opportunity to suggest a couple of improvements in your PR. It’s totally optional, feel free to ignore my comment, that’s why I didn’t mark it as “Requested changes.”
There was a problem hiding this comment.
No worries, I get it, and thank you for the review :) It just felt a little out of place.
| local url=$3 | ||
| local max_attempts=${4:-30} | ||
| local wait_seconds=${5:-30} | ||
| local artifacts_subdir=$4 |
There was a problem hiding this comment.
This is a breaking positional parameter change — old $4 was max_attempts (optional), now it's artifacts_subdir (required). The in-repo caller (testing::check_and_test) is updated, but if any downstream scripts or forks call testing::check_backstage_running with the old positional args, they'd silently break (passing a number as artifacts_subdir). Just flagging the risk.
There was a problem hiding this comment.
It's called only once (in testing::check_and_test), I don't believe there will be any downstream scripts, or forks that will suffer from it (you either use the old version, or you rebase and use the new version).
It may make cherry-picking slightly harder, but the function has been moved to the testing:: library, so it doesn't matter.
On the other hand, it's a much more logical order.
| helm::merge_values "merge" "${DIR}/value_files/${HELM_CHART_VALUE_FILE_NAME}" "${DIR}/value_files/${HELM_CHART_OSD_GCP_DIFF_VALUE_FILE_NAME}" "/tmp/merged-values_showcase_OSD-GCP.yaml" | ||
| mkdir -p "${ARTIFACT_DIR}/${NAME_SPACE}" | ||
| rsync -a "/tmp/merged-values_showcase_OSD-GCP.yaml" "${ARTIFACT_DIR}/${NAME_SPACE}/" # Save the final value-file into the artifacts directory. | ||
| common::save_artifact "${artifacts_subdir}" "/tmp/merged-values_showcase_OSD-GCP.yaml" |
There was a problem hiding this comment.
Most other common::save_artifact calls in this PR have || true to prevent artifact-save failures from aborting the pipeline, but this one doesn't. The original rsync it replaced also lacked it, but since you're cleaning this up anyway — should it match the others for consistency?
There was a problem hiding this comment.
In the codebase the collection of merged yaml value files files (like this one) lack the || true, because the files should always be created and if they aren't the pipeline should fail anyway. And if the artifact collection is unsuccessful, it's making it even harder to investigate. I only see the || true is useful only if the files may not be created, which is rare in the codebase. I haven't seen a failure because of artifact collection which wasn't connected to another issue
|
|
/lgtm |
83d28ef
into
redhat-developer:main



Description
This PR fixes several CI and E2E test issues for Operator-backed deployments:
{{inherit}}in Operator dynamic plugins YAML: Replaces{{ "{{"}}inherit{{ "}}" }}Helm escaping with plain{{inherit}}in the dynamic plugins file dynamically generated for Operator deployments. This is a quick fix — long-term solution involves splitting value files from dynamic plugins definitions.save_artifacthelper function where possible.BACKSTAGE_DEPLOY_SELECTOR) for both Helm and Operator deployments, replacing hardcoded pod selectors. Pod selectors are now derived from deploymentmatchLabelsinstead of being hardcoded..gitignore: Add.local-testto gitignore (it was interfering with pre-commit hooks).prepare_operatorcall, deduplicate pod selector logic inkube-client.ts, and remove unused variables.It doesn't fix every one of the failing test cases, but vastly improves the current situation across both OCP and K8s platforms. Fixing everything would require spending even more time and over-inflating this PR.
Which issue(s) does this PR fix
PR acceptance criteria
Please make sure that the following steps are complete:
How to test changes / Special notes to the reviewer